-
Notifications
You must be signed in to change notification settings - Fork 127
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support for working with markdown front matter #642
Conversation
Fixes #641 Add support for working with markdown front matter. * **packages/core/src/frontmatter.ts** - Add `splitMarkdown` function to extract frontmatter and markdown content. - Add `updateFrontmatter` function to update frontmatter with new content. - Ensure `splitMarkdown` and `updateFrontmatter` handle various formats (yaml, json, toml). * **packages/core/src/frontmatter.test.ts** - Add tests for `splitMarkdown` function. - Add tests for `updateFrontmatter` function. * **packages/core/src/markdown.ts** - Import `splitMarkdown` and `updateFrontmatter` from `frontmatter.ts`. - Add `mergeFrontmatter` function to split markdown and update frontmatter. - Ensure `mergeFrontmatter` handles various formats (yaml, json, toml). --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/microsoft/genaiscript/issues/641?shareId=XXXX-XXXX-XXXX-XXXX).
The changes in this pull request mainly entail the implementation of functionality for parsing, splitting, and updating front matter in markdown files. The front matter can be in the format of YAML, JSON, or TOML.
The changes look good in general but I have a few concerns:
Apart from these, the code looks good for merging. LGTM 🚀
|
packages/core/src/markdown.ts
Outdated
): string { | ||
const { frontmatter, content } = splitMarkdown(text, options) | ||
const updatedFrontmatter = { ...frontmatter, ...newFrontmatter } | ||
return updateFrontmatter(text, updatedFrontmatter, options) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no null check for the splitMarkdown
function result. If this function returns null or undefined, it could lead to a runtime error when trying to destructure frontmatter
and content
. Consider adding a null check before destructuring the result. 🛠️
generated by pr-review-commit
missing_null_check
break | ||
default: | ||
res = YAMLTryParse(fm) | ||
break | ||
throw new Error(`Unsupported format: ${format}`) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The default case in the switch statement throws an error for unsupported formats. However, the function does not handle this error, which could lead to unhandled exceptions. Consider providing a default format or handling this error in a way that does not interrupt the execution of the program. 😊
generated by pr-review-commit
unsupported_format
frontmatterTryParse(text, { format })?.value ?? {}, | ||
content: (text) => splitMarkdown(text)?.content, | ||
updateFrontmatter: (text, frontmatter, format): string => | ||
updateFrontmatter(text, frontmatter, { format }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function updateFrontmatter
is called without error handling. If this function throws an error, it could lead to an unhandled exception. Consider adding error handling to improve the robustness of your code. 😊
generated by pr-review-commit
missing_error_handling
|
||
## `frontmatter` | ||
|
||
Extracts and parses the frontmatter text from a markdown file. Returns `undefined` if no frontmatter is not found or the parsing failed. Default format is `yaml`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There seems to be a typo here. "no frontmatter is not found" should be "no frontmatter is found".
generated by pr-docs-review-commit
typo
@@ -129,7 +129,7 @@ title: "Hello, World!" | |||
... | |||
``` | |||
|
|||
You can use the `parsers.frontmatter` to parse out the metadata into an object | |||
You can use the `parsers.frontmatter` or [MD](./md.md) to parse out the metadata into an object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The relative link to MD documentation should be verified to ensure it correctly points to the md.md file within the same directory.
generated by pr-docs-review-commit
relative_link
text: string, | ||
frontmatter: any, | ||
format?: "yaml" | "json" | ||
): string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The parameter descriptions for the updateFrontmatter
method are missing. It would be helpful to add descriptions for text
, frontmatter
, and format
to provide more context for developers. 😇
generated by pr-review-commit
missing_param_description
@@ -129,7 +129,7 @@ title: "Hello, World!" | |||
... | |||
``` | |||
|
|||
You can use the `parsers.frontmatter` to parse out the metadata into an object | |||
You can use the `parsers.frontmatter` or [MD](./md.md) to parse out the metadata into an object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The link to MD in "MD" is incorrect. It should be "MD" without the "./" since it's in the same directory.
generated by pr-docs-review-commit
incorrect_link
delete frontmatter[key] | ||
} else { | ||
frontmatter[key] = value | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code deletes a key from the frontmatter
object if the value is null. This could lead to unexpected behavior if other parts of the code rely on this key existing. Consider setting the key to null
or undefined
instead of deleting it.
generated by pr-review-commit
delete_key
Fixes #641
Add support for working with markdown front matter.
packages/core/src/frontmatter.ts
splitMarkdown
function to extract frontmatter and markdown content.updateFrontmatter
function to update frontmatter with new content.splitMarkdown
andupdateFrontmatter
handle various formats (yaml, json, toml).packages/core/src/frontmatter.test.ts
splitMarkdown
function.updateFrontmatter
function.packages/core/src/markdown.ts
splitMarkdown
andupdateFrontmatter
fromfrontmatter.ts
.mergeFrontmatter
function to split markdown and update frontmatter.mergeFrontmatter
handles various formats (yaml, json, toml).For more details, open the Copilot Workspace session.
The key changes in this GIT_DIFF focused on two areas: unit tests for various functions and changes in functionality.
New unit tests are added for the functions
splitMarkdown
andupdateFrontmatter
. These tests ensure that the functions work as expected with different formats of frontmatter (yaml, json, and toml) in markdown files.The function
splitMarkdown
was introduced and it takes a string of text and an optional format (yaml, json, or toml). It divides the text into its frontmatter and content sections based on the provided format.The function
updateFrontmatter
was introduced and it updates the existing frontmatter of a given markdown text with a new one. If no format is specified, it defaults to yaml.Modifications to the
frontmatterTryParse
function have been made, although specific changes are slightly unclear from the diff.The
mergeFrontmatter
function was added to the Markdown file. It combines the original frontmatter with the new one and updates it in the text, using the aforementionedsplitMarkdown
andupdateFrontmatter
functions.We also see the inclusion of
splitMarkdown
andupdateFrontmatter
functions intosrc/markdown.ts
Key Changes:
frontmatterTryParse
,splitMarkdown
, andupdateFrontmatter
functions are all performing well.splitMarkdown
andupdateFrontmatter
.frontmatterTryParse
function got an update, achieving more functionality.src/markdown.ts
. The power of frontmatter manipulation right in your Markdown!Please note - no user-facing changes were made as no changes were seen in "packages/core/src/prompt_template.d.ts" and "packages/core/src/prompt_type.ts".